-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
A simple Json implementation for future use. #4708
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an extraction of json parser from #4683 ?
@trams Yes. I need to split that PR into a few dependent PRs. I will address your comment from that PR here. :-). Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM in general.
Some questions:
- Why specialize the file reader based on os?
- How is this implementation different from that provided by dmlc
- Why is the use of friend classes necessary? (I always ask this question as friend classes commonly indicate a shortcut instead or proper oo design)
@RAMitchell Removed the friend class. |
Since it doesn't modify existing code base, I incline to merge it here with all related comments addressed as now I'm working on the cudf intergration which will make use of this Json implementation. Any objection? @hcho3 @CodingCat |
This will enable #4702 and #4683 (extracted from there). Other than serialization, the JSon implementation will be used to consume numpy array interface, and cuda array interface. This will benefit XGBoost as creating the generic interface will enable introp with lots of existing libraries (
arrow and friends, cupy, numba etc..) without creating specific C backend source code.A json implementation will the first part of on going effort for above described features. This PR doesn't touch any existing code base, can we consider merging it although it's not used by other code?